Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: signature help #396

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

lucacervello
Copy link
Contributor

@lucacervello lucacervello commented Mar 12, 2024

I've reduced the scope of this MR.

I've reused the Definition code and showed the signature.

It doesn't work while you're writing code like String.replace( and it doesn't show the active parameter.

Screen.Recording.2024-03-18.at.17.15.43.mov

@mhanberg
Copy link
Collaborator

mhanberg commented Mar 13, 2024

Also I had the thought, this needs to work for standard library and Erlang functions as well. Which those aren't* traced by the compiler cuz they're already compiled.

For those, I believe will need to read them from the runtime in some way (I'll leave you to figure that out)

I think we can ship a first version without that tho

Edit: typo "are" -> "aren't"

@lucacervello lucacervello force-pushed the signature-help branch 3 times, most recently from c4df7ba to c693292 Compare March 18, 2024 16:29
@lucacervello lucacervello marked this pull request as ready for review March 18, 2024 16:59
@mhanberg
Copy link
Collaborator

Played with it and realized that I'm not sure this path forward will work.

this searched for the definition of the module/function before the cursor, but that implies the project has compiled and its in the references table.

I think want we actually want to do is extract the module and function before the cursor and then fetch the docs with Code.fetch_docs/1

that should return the function docs which will actually have a "signature" field that we can use

CleanShot 2024-03-26 at 15 10 59@2x

@lucacervello
Copy link
Contributor Author

@mhanberg I switched approach.

It works well for something like

Enum.map(list, fun)

It doesn't work for aliased or imported functions like

alias NextLS.Formatter

Formatter.format(code)

Because Code.fetch_docs doesn't find Formatter

@mhanberg
Copy link
Collaborator

Gotcha, that should become available as I build out the "buffer environment".

I'll try this out today

@lucacervello
Copy link
Contributor Author

Gotcha, that should become available as I build out the "buffer environment".

I'll try this out today

Thank you, if you're happy with that limitation, I can cleanup the code later 👍

@mhanberg
Copy link
Collaborator

Yeah the limitation is fine for now, it's basically already a limitation for autocomplete.

lib/next_ls/signature_help.ex Outdated Show resolved Hide resolved
lib/next_ls/signature_help.ex Outdated Show resolved Hide resolved
lib/next_ls/signature_help.ex Outdated Show resolved Hide resolved
lib/next_ls/signature_help.ex Outdated Show resolved Hide resolved
lib/next_ls/signature_help.ex Outdated Show resolved Hide resolved
@mhanberg
Copy link
Collaborator

mhanberg commented Apr 4, 2024

Also, I think we should go ahead and make the active parameter work before merging.

I imagine you can take the ast you find and then find the parameter the cursor is inside and use that to determine the index to use.

@lucacervello
Copy link
Contributor Author

@mhanberg I added the active parameter, tests are working but I'm having trouble using it in my editor. I'll investigate more later.

@mhanberg
Copy link
Collaborator

CleanShot 2024-04-12 at 13 19 19@2x
can confirm that the active parameter seems to not be working in the editor, but i rebased on main (i just merged a big change #410 ) and it works other than that, for stdlib, deps, and project functions

@mhanberg
Copy link
Collaborator

I think i need to make a patch to spitfire for this to work the way its currently coded.

@mhanberg
Copy link
Collaborator

I was re reading the PR comments and I can't seem to remember what patch I needed to make to spitfire.

@mhanberg mhanberg force-pushed the main branch 2 times, most recently from 06acca3 to f4685d0 Compare May 15, 2024 14:13
@mhanberg
Copy link
Collaborator

Now spitfire has the function Spitfire.container_cursor_to_quoted, which is used to insert a __cursor__() marker in the ast where the cursor is.

I think what you can do here is use that function, create a zipper, find the cursor, then traverse up until you see the first function call, and that should be the function that should be used for signature help.

you might need to also "expand" the ast at the cursor like we do for completions to get the environment, which should help you resolve imports and aliases for that function.

@lucacervello
Copy link
Contributor Author

Now spitfire has the function Spitfire.container_cursor_to_quoted, which is used to insert a __cursor__() marker in the ast where the cursor is.

This could work but only for partial code like

Enum.map(

I think we also want to give SignatureHelp when the cursor is on valid code like

Enum.map([1, 2], fn n -> n + 1 end)

@mhanberg
Copy link
Collaborator

Now spitfire has the function Spitfire.container_cursor_to_quoted, which is used to insert a __cursor__() marker in the ast where the cursor is.

This could work but only for partial code like

Enum.map(

I think we also want to give SignatureHelp when the cursor is on valid code like

Enum.map([1, 2], fn n -> n + 1 end)

You make it into a partial code snippet by truncating the document after the cursor position.

Basically if the cursor is the location line: 0, character: 30, you'd truncate it too look like Enum.map([1, 2], fn n -> n + and then run the following operations, which produces ast you can use

iex(2)> {:ok, ast} = Spitfire.container_cursor_to_quoted("Enum.map([1, 2], fn n -> n + ")
{:ok,
 {{:., [line: 1, column: 5],
   [
     {:__aliases__, [last: [line: 1, column: 1], line: 1, column: 1], [:Enum]},
     :map
   ]}, [closing: [line: 1, column: 45], line: 1, column: 6],
  [
    [1, 2],
    {:fn, [closing: [line: 1, column: 42], line: 1, column: 18],
     [
       {:->, [line: 1, column: 23],
        [
          [{:n, [line: 1, column: 21], nil}],
          {:+, [line: 1, column: 28],
           [
             {:n, [line: 1, column: 26], nil},
             {:__cursor__,
              [closing: [line: 1, column: 41], line: 1, column: 30], []}
           ]}
        ]}
     ]}
  ]}}
iex(3)> ast |> Macro.to_string() |> IO.puts
Enum.map([1, 2], fn n -> n + __cursor__() end)

@lucacervello
Copy link
Contributor Author

You make it into a partial code snippet by truncating the document after the cursor position.

Oh right, It make a lot of sense, dumb question, my bad 😅. Thank you.

I hope I have some time in the upcoming weeks to push this PR forward 🤞

@mhanberg
Copy link
Collaborator

You make it into a partial code snippet by truncating the document after the cursor position.

Oh right, It make a lot of sense, dumb question, my bad 😅. Thank you.

I hope I have some time in the upcoming weeks to push this PR forward 🤞

No worries, its all volunteer work! Work on it when you have the time, energy, and interest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants